iOS 向けに AudioTrackSink を追加する#128
Conversation
|
!haiku レビューして |
This comment was marked as resolved.
This comment was marked as resolved.
|
!haiku レビューして |
This comment was marked as resolved.
This comment was marked as resolved.
|
!haiku 以下の観点でレビューして
|
This comment was marked as resolved.
This comment was marked as resolved.
| steps: | ||
| - uses: actions/checkout@v5 | ||
| - name: Select Xcode 16.1 | ||
| - name: Select Xcode 26.0 |
| + | ||
| +/** | ||
| + * このアダプターが公開するネイティブの AudioTrackSinkInterface。呼び出しは生成時に | ||
| + * 渡された RTCAudioTrackSink へ変換されます。このポインタは非安全で、このクラスが所有します。 |
There was a problem hiding this comment.
「呼び出しは生成時に渡された RTCAudioTrackSink へ変換されます。」の意味が全く理解できないので、もうちょっとコメント考えてほしいです。
「このポインタは非安全で、このクラスが所有します。」も、非安全という書き方もあまり見かけないし、非安全がどういう意味なのかが分からないです。
There was a problem hiding this comment.
実態は AudioTrackSinkAdapter を見ればわかると思ったので、ここもコメント削除します
| + * webrtc::AudioTrackSinkInterface は WebRTC の音声処理で使われ、このアダプターが | ||
| + * その呼び出しを生成時に渡された RTCAudioTrackSink へ橋渡しします。 | ||
| + */ | ||
| +@interface RTCAudioTrackSinkAdapter : NSObject |
There was a problem hiding this comment.
このクラスは公開する必要なさそうなので、このファイルは不要そうに見えます
| - (instancetype)init NS_UNAVAILABLE; | ||
|
|
||
| -/** The audio source for this audio track. */ | ||
| +/** このオーディオトラックの音声ソース。 */ |
| } | ||
|
|
||
| func preferredNumberOfChannels() -> Int { | ||
| return 1 // 例: モノラルで受けたい場合。指定しなければ -1 を返す。 |
There was a problem hiding this comment.
指定しなければ -1 を返す
「実装しなければ -1 を返すデフォルト実装が使われる」
「特にチャンネル数を指定しない場合は -1 を返しても構わない」
あたりかな。ただ後者の書き方だとこの関数の実装をサボれることが分からないので、前者寄りの書き方が良さそうな気がします。
There was a problem hiding this comment.
ありがとうございます。前者の書き方に修正します。
|
|
||
| ### パッチ実装のポイント | ||
|
|
||
| - `RTCAudioTrackSinkAdapter` は `AudioTrackSinkInterface` を実装し、`OnData` で受け取った PCM を `NSData` にコピーして `RTCAudioTrackSink#onData` に渡す。コールバックはネイティブ音声スレッドで呼ばれるため、重い処理は別スレッドへ委譲する必要があることに注意。 |
There was a problem hiding this comment.
RTCAudioTrackSinkAdapterはAudioTrackSinkInterfaceを実装し
AudioTrackSinkInterface を実装しているのは AudioTrackSinkAdapter のはず。
RTCAudioTrackSink#onDataに渡す
# で繋ぐのは Java の書き方なので、Objective-C 的には「RTCAudioTrackSink の onData:bitsPerSample:sampleRate:numberOfChannels:numberOfFrames: に渡す」かな。
別スレッドへ委譲する必要がある、に関してはパッチ実装のポイントではないので不要そう。
There was a problem hiding this comment.
別スレッドへ〜の説明はドキュメント下の注意点の説明の部分に移動させました。
| + | ||
| +/** | ||
| + * このアダプター生成時に渡された Objective-C の音声トラックシンク。 | ||
| + * webrtc::AudioTrackSinkInterface への呼び出しはここへ変換されて渡されます。 |
There was a problem hiding this comment.
この行のコメント理解するのにめっちゃ時間掛かったので、直せそうなら直して欲しいかも。
そもそもこの説明を書く必要あるかが疑問。
There was a problem hiding this comment.
header file の方で使われ方について実装も交えたコメントをわざわざ書くのもおかしいと思ったので、説明を削除しました
| + return static_cast<int>([sink preferredNumberOfChannels]); | ||
| + } | ||
| + return -1; | ||
| + } |
There was a problem hiding this comment.
修正しました。(上の int Num ~ の先頭に合わせました
Co-authored-by: melpon <shigemasa7watanabe+github@gmail.com>
Co-authored-by: melpon <shigemasa7watanabe+github@gmail.com>
| + if ([sink respondsToSelector:@selector(preferredNumberOfChannels)]) { | ||
| + return static_cast<int>([sink preferredNumberOfChannels]); | ||
| + } | ||
| + return -1; |
There was a problem hiding this comment.
インデントが2スペースであることを考えると、このあたりのインデントも直す必要がありそう。
というより、depot_tools にパスを通してから git cl format しても良いと思います。
There was a problem hiding this comment.
PATH を通したあとに、
python3 run.py revert --patch ios_audio_track_sink.patch
cd _source/ios_sdk/webrtc/src
# 新しいファイルは一度ステージングする
# git add sdk/objc/api/RTCAudioTrackSinkAdapter.mm
git cl format [file 指定]
# ステージング解除
# git restore --staged sdk/objc/api/RTCAudioTrackSinkAdapter.mm
python3 run.py diff ios_sdk > patches/ios_audio_track_sink.patchこの手順でフォーマットをかけて、パッチを更新してみます。
単純に git cl format を実行すると全く関係のないファイルまでフォーマットされたので、とりあえず必要なファイルにだけフォーマットをあてるスタイルです。
Co-authored-by: melpon <shigemasa7watanabe+github@gmail.com>
Co-authored-by: melpon <shigemasa7watanabe+github@gmail.com>
No description provided.